Skip to content

[3.5] bpo-30458: Disallow control chars in http URLs. (GH-12755)#13207

Merged
larryhastings merged 4 commits intopython:3.5from
hroncok:3.5-issue30458
Jul 14, 2019
Merged

[3.5] bpo-30458: Disallow control chars in http URLs. (GH-12755)#13207
larryhastings merged 4 commits intopython:3.5from
hroncok:3.5-issue30458

Conversation

@hroncok
Copy link
Contributor

@hroncok hroncok commented May 8, 2019 •
edited by bedevere-bot
Loading

Disallow control chars in http URLs in urllib.urlopen. This addresses a potential security problem for applications that do not sanity check their URLs where http request headers could be injected.

Disable https related urllib tests on a build without ssl (GH-13032)
These tests require an SSL enabled build. Skip these tests when python is built without SSL to fix test failures.

Use http.client.InvalidURL instead of ValueError as the new error case's exception. (GH-13044)

Co-Authored-By: Miro Hrončok miro@hroncok.cz

https://bugs.python.org/issue30458

@gpshead @hroncok
bpo-30458: Disallow control chars in http URLs. (pythonGH-12755)
d46584b 
Disallow control chars in http URLs in urllib.urlopen.  This addresses a potential security problem for applications that do not sanity check their URLs where http request headers could be injected.

Disable https related urllib tests on a build without ssl (pythonGH-13032)
These tests require an SSL enabled build. Skip these tests when python is built without SSL to fix test failures.

Use http.client.InvalidURL instead of ValueError as the new error case's exception. (pythonGH-13044)

Co-Authored-By: Miro Hrončok <miro@hroncok.cz>
@bedevere-bot bedevere-bot added type-bug An unexpected behavior, bug, or error type-security A security issue labels May 8, 2019
@vstinner
Copy link
Member

vstinner commented May 21, 2019 •
edited
Loading

Multiple tests failed on AppVeyor:

ERROR: test_networked_good_cert (test.test_httplib.HTTPSTest)
ERROR: test_connect (test.test_ssl.NetworkedTests)
ERROR: test_connect_cadata (test.test_ssl.NetworkedTests)
ERROR: test_handshake (test.test_ssl.NetworkedBIOTests)
...

With TLS certification validation error:

[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed (_ssl.c:728)

It seems like these failures are related to https://bugs.python.org/issue36816: PR #13200 isn't merged yet.

@vstinner
Copy link
Member

vstinner commented May 21, 2019

It seems like these failures are related to https://bugs.python.org/issue36816: PR #13200 isn't merged yet.

Travis CI basically has the same failures and so it's likely the same issue.

vstinner
vstinner approved these changes May 21, 2019
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Straightforward backport (cherry-pick). The patch went fine in all other branches.

@hroncok
Copy link
Contributor Author

hroncok commented May 21, 2019

Compared to 3.6, I've removed the f-strings.

@tirkarthi tirkarthi mentioned this pull request Jun 3, 2019
Closed
@larryhastings larryhastings merged commit afe3a49 into python:3.5 Jul 14, 2019
@bedevere-bot
Copy link

bedevere-bot commented Jul 14, 2019

@larryhastings: Please replace # with GH- in the commit message next time. Thanks!

@larryhastings
Copy link
Contributor

larryhastings commented Jul 14, 2019

Thanks for the 3.5 love!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes

Assignees

No one assigned

Labels

type-bug An unexpected behavior, bug, or error type-security A security issue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@hroncok @vstinner @bedevere-bot @larryhastings @the-knights-who-say-ni @gpshead